TOMEE-4655 - roll back a failed deployment's registered ids by jungm · Pull Request #2850 · apache/tomee

jungm · 2026-07-23T19:24:38Z

What

Fixes TOMEE-4655: a failed CDI/EJB deployment leaks its deployment id into later apps.

Why

Assembler.createApplication(AppInfo, ClassLoader, boolean) registers every EJB's deployment id in the ContainerSystem (via initEjbs) before it starts CDI. Its catch block treated two exception types specially:

} catch (final ValidationException | DeploymentException ve) {
    throw ve;                        // no cleanup
} catch (final Throwable t) {
    destroyApplication(appInfo);     // cleanup
    ...
}

CDI startup failures bubble up as jakarta.enterprise.inject.spi.DeploymentException, so they hit the first clause and returned without undeploying the partially deployed application. Every BeanContext registered before the CDI phase stayed in CoreContainerSystem.deployments. The next app reusing one of those ids then tripped getDuplicates → DuplicateDeploymentIdException before any of its own lifecycle/managed-bean/concurrency checks ran — turning one bad deployment into a cascade of unrelated failures.

The git history shows that clause was only ever added so these two exception types wouldn't be wrapped in an OpenEJBException (commit 51ab12b2, "as ValidationException, DeploymentException shouldn't be wrapped"). Losing the rollback was an accidental side effect, not the intent.

The fix

Run destroyApplication(appInfo) on this path as well, then rethrow the original exception unchanged — preserving the "don't wrap" intent while releasing the ids.

Testing

New FailedDeploymentIdCleanupTest deploys an app whose singleton has an unsatisfiable @Inject (failing at CDI start), then asserts a second app can reuse the same deployment id. Verified it has real diagnostic power:

Without the fix: AssertionError: the failed deployment leaked its deployment id expected null, but was:<BeanContext(id=TheSharedDeploymentId)>
With the fix: passes
No regressions across the neighbouring assembler.classic tests (RedeployTest, EjbRefTest, etc.)

Also verified end-to-end against the Jakarta EE 11 Web Profile TCK (enterprise-beans-30 partition, Plume): 1138 tests, 0 failures, 0 errors, and zero DuplicateDeploymentIdException. The four excluded test patterns the ticket calls out now deploy cleanly.

Note for reviewers

The matching TCK-harness changes (removing the exclusions, updating the expected class count, and KNOWN_FAILURES.md) live in the separate apache/tomee-tck repo and will be submitted there. Two JsfClientEjblitejsfTest classes among those exclusions still fail after this fix, but for an unrelated, already-documented reason — the OpenWebBeans "cannot proxy a final method" interceptor gap, which the deployment-id leak had been masking. They are moved into that existing exclusion block rather than left enabled.

🤖 Generated with Claude Code

When a CDI or EJB deployment failed, Assembler.createApplication rethrew ValidationException/DeploymentException without undeploying the partially deployed application. CDI startup failures surface as a jakarta.enterprise.inject.spi.DeploymentException, so they took this path and left every already-registered BeanContext in the ContainerSystem. The next application reusing one of those deployment ids then failed with a DuplicateDeploymentIdException before reaching its own checks, turning one bad deployment into a cascade of unrelated failures. That catch clause only ever existed so these two exception types would not be wrapped in an OpenEJBException; skipping the rollback was an unintended side effect. Run destroyApplication(appInfo) on this path as well, then rethrow the original exception unchanged. Adds FailedDeploymentIdCleanupTest, which fails against the previous code with the exact "leaked its deployment id" symptom and passes with the fix.

rzo1 · 2026-07-27T08:13:23Z

Correct diagnosis and the minimal fix. Confirmed the chain: the unwrapped
DeploymentException really does come from ThreadSingletonServiceImpl:259, initEjbs
really does register ids before new CdiBuilder().build(...), and the message key
createApplication.undeployFailed already exists at Messages.properties:76. Ran the
test both ways — unpatched it fails with
the failed deployment leaked its deployment id expected null, but was:<BeanContext(id=TheSharedDeploymentId)>.

One thing I'd like fixed before merge, which my local run surfaced:

Rolling back a failure that happens before startEjbs now drives
Container.stop/undeploy over BeanContexts that were never deployed into their
container, which emits ERROR-level NPEs (e.g. SingletonInstanceManager:216) for every
singleton/stateful bean. Since a CDI bootstrap failure is now the most common path
through this branch, that means the common failure mode gets a wall of ERROR-level
noise stacked on top of the real cause. Guarding destroyApplication on whether
startEjbs ran, or making the container stop tolerant of an undeployed BeanContext,
would fix it.

And one I couldn't prove but would want smoke-tested:

For webapps, destroyApplication calls webAppBuilder.undeployWebApps(appInfo), which
ends in host.removeChild(standardContext) (TomcatWebAppBuilder:1663-1690), plus
ClassLoaderUtil.destroyClassLoader. createApplication for a webapp runs on the
Tomcat context-start thread, and TomcatWebAppBuilder's own catch at :1341 then calls
undeploy(...) again. So this newly routes the most common webapp deployment failure
into an in-flight Catalina child removal plus classloader destruction, followed by a
second undeploy from the caller.

The pre-existing catch (Throwable) branch already does exactly this for other
failures and the second undeploy looks idempotent (findChild returns null), so I
suspect it's fine — but it's the one behavioural change here that reaches outside
openejb-core. Could you deploy a war with an unsatisfied @Inject into a real TomEE
and confirm the log is clean? The openejb-core unit test can't see this.

Nit: catch (final Exception expected) in the test is broad enough that it would still
pass if the deployment started failing for an unrelated reason. Asserting on
DeploymentException would keep it guarding the branch you actually fixed.

The rollback added for TOMEE-4655 runs over BeanContexts that startEjbs never deployed into their container, because a CDI bootstrap failure happens before startEjbs. EjbJarBuilder assigns the container at build time while the container data only appears in Container.deploy, so the stop/undeploy loops hit a null Data and threw a NullPointerException for every singleton and stateful bean. Those were collected into the UndeployException and logged, burying the real cause under a wall of errors on what is now the most common path through this branch. Guard the two containers that lacked the check that ManagedContainer, StatelessInstanceManager and SingletonInstanceManager.undeploy already have, and skip a bean whose container was already cleared. Also tighten the test to catch DeploymentException rather than Exception, so it keeps guarding the branch it was written for, and add a case that undeploys a bean which was never deployed.

jungm · 2026-07-28T19:48:00Z

Thanks for the careful review — both points confirmed, and both are addressed in e4e60f8.

1. ERROR-level NPE noise on the pre-startEjbs rollback — fixed.

Reproduced exactly as you described. The root cause is an asymmetry: EjbJarBuilder:83 assigns the container at build time, while containerData is only set inside Container.deploy(). So after a CDI failure the rollback holds a non-null container for a bean the container never saw, and SingletonInstanceManager.freeInstance:216 (data.singleton) plus StatefulContainer.undeploy:292 (data.jmxNames) both dereference a null Data.

Notably SingletonInstanceManager.undeploy:341, StatelessInstanceManager.undeploy and ManagedContainer.undeploy already have precisely this guard — these two were simply the ones that lacked it, so the fix follows the existing idiom rather than inventing one. I swept every Container.undeploy implementation; no others are missing it.

A third variant showed up once those were guarded: Assembler's stop/undeploy loops NPE'd on getContainer() itself for beans whose container had already been cleared, so those are null-checked too. The rollback is now completely silent — 0 NPEs.

2. Webapp smoke test — done, and it is not a regression.

Deployed a war with an unsatisfied @Inject (plus a stateful and a singleton bean) into a real TomEE Plus, then ran the same war against a pristine pre-fix openejb-core for a controlled A/B on freshly unpacked servers:

	pristine	with fix
deploy attempts	1	1
`NullPointerException`	0	0
`Error destroying child`	1	1
`invalid Lifecycle transition`	1	1
SEVERE total	4	4

The normalized SEVERE lines are identical between the two; only their ordering shifts, since the rollback now runs slightly earlier relative to TomcatWebAppBuilder's own message. The removeChild/before_destroy interaction you spotted is real, but it is pre-existing upstream behaviour on the webapp path — TomcatWebAppBuilder's catch at :1341 already undeploys before the caller does, and the second undeploy is idempotent as you suspected. The server stays healthy afterwards (/ returns 200, the failed app is correctly absent). Worth noting I first measured 2× these counts and had to discard that run — a stale exploded dir in work/temp had caused the war to deploy twice, so the control was invalid rather than the fix.

3. Nit — fixed. The test now catches DeploymentException rather than Exception, so it still guards the branch it was written for. Added undeployingABeanThatWasNeverDeployedIsQuiet, which fails without the container guards with NullPointerException: Cannot read field "jmxNames" because "data" is null and passes with them.

One thing to flag: StatefulDecoratorInjectionTest, StatefulConversationScopedTOMEE1138Test and StatefulDependentInjectionTest fail in openejb-core, but they fail identically on pristine a3e8806a9d with no changes applied, so they are unrelated to this PR.

🤖 Addressed by Claude Code

rzo1 approved these changes Jul 29, 2026

View reviewed changes

jungm merged commit 68913b7 into apache:main Jul 31, 2026
1 check passed

jungm deleted the claude/tomee-4655-fix-6bffa1 branch July 31, 2026 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

TOMEE-4655 - roll back a failed deployment's registered ids - #2850

TOMEE-4655 - roll back a failed deployment's registered ids#2850
jungm merged 2 commits into
apache:mainfrom
jungm:claude/tomee-4655-fix-6bffa1

jungm commented Jul 23, 2026

Uh oh!

rzo1 commented Jul 27, 2026

Uh oh!

jungm commented Jul 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants